[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

✅ Deploy Preview for kubernetes-ingress-nginx ready!

Name	Link
🔨 Latest commit	cfda5f8
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66c90880874f9d000838e2c8
😎 Deploy Preview	https://deploy-preview-11851--kubernetes-ingress-nginx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

/assign @strongjz @aojea

please do not cherry-pick this change :)

Any recommendation for people who use ingress-nginx as the fronting load balancer? :)

I think the infra-provider created LoadBalancers support rate-limiting on most cloud providers.

Sad fact: vSphere works with MetalLB, so there is no such Load Balancer fronting your cluster. The Load Balancer IP is a virtual IP of a node. Just one example...

True. But also practical aspect is Metallb/On-premise type engineers will make sophisticated choices like rate-limit or DDOS protection etc on their edge router.

What this PR does / why we need it:

This PR removes the global-rate-limit feature. On the goal to make ingress-nginx more slim, we need to deprecate features not widely used.

Specifically this feature should be controlled by the fronting Load Balancer and not by ingress-nginx, and it adds some complexity on the code that we may not be able to maintain in future releases

/kind deprecation

How do you know this feature isn't widely used?

There are no easy answers for a global-rate-limit for bare-metal or on-prem setups. Especially when you look at l7-aware rate-limiting, or internal rate limiting.

Cilium and MetalLB LoadBalancers do not support the same features, and this removes a pretty significant function from ingress-nginx. I think this is a mistake.

What would it take to add this back? A fork or internal (to ingress-nginx) implementation?

Wait, what evidence do you have that this feature is not widely used? Because I can tell you, in my org, it is.
Where did the "goal to make ingress-nginx more slim" come from?
Why is this a goal?
Are people complaining that the ingress-controller is "too big"?
If this is a goal, why is the community not involved in the discussion on what features to cut and which get to stay?

The maintainers understand that this is a breaking change.

Attempts have been made to communicate the reasons leading up to this and other changes.

• There is lack of resources to dedicate for supporting and maintaining features that are relatively distant from the specs of the core Ingress-API KEP
• While there were resources, a whole lot of features were maintained and supported over several years but now there is acute shortage of developers dedicated to maintaining the features that are not implied by the core Ingress-API specs
• The forward looking work is to implement the Gateway-API and that brings in many features currently requested/expected
• Contributions are most welcome but the project has to face the complexity of being unable to sustain the one time contributions that required sustained maintenance, because some of the contributors are not available as long term maintainers
• To a small extent the Nginx based rate-limiting is such a useful feature for reasons mentioned in other posts here. But it is also true that in certain specific use cases, it is better to completely avoid traffic that can be categorized as undesired traffic. For example a flood (DDOS) is better handled on the edge so that the controller pods don't go into overdrive for cpu/memory/bandwidth usage

We have:

• a Slack channel that we announce also the deprecations
• a bi-weekly meeting
• a mailing list (ingress-nginx-dev)
• a permanent issue (Permanent issue - Breaking changes documentations #10186) containing breaking changes

Still it seems to not be enough to let people know we have some actions in place to simplify ingress-nginx

We have a long term running project to simplify ingress-nginx for a simple reason: today is unsustainable. We got multiple CVEs coming from multiple different features and while we've been asked by the users to add (and keep) more features, we've also been asked by k8s leadership to take actions to fix and simplify these kind of problems.

And yes, some hard to swallow pills should be taken to increase project quality, including deprecating features.

There is no restriction on this project being forked, or internally maintained, or if your company is willing to support you on being an opensource contributor for us to accept your contributions but, being clear, we are limiting now to bugfixes and security improvements. So if you decide going to the fork/private repo and find some bug/issue on main code, please remember so far this project existed by volunteers contributions.

Thanks

+100 to what the maintainers say, I'm completely sure that people want the features, but that they also want well maintained code without bugs, and foremost without CVEs, because if your company is hacked because the ingress-nginx that you are running has security issues, the project is to blame.

What would it take to add this back? A fork or internal (to ingress-nginx) implementation?

This code is under Apache license, forking is always an option

Still it seems to not be enough to let people know we have some actions in place to simplify ingress-nginx

I would recommend updating the documentation site on every page with a banner warning about upcoming changes, as well as include warnings in the release notes.

It would also be nice to have deprecation warnings in any patch releases to give users time to update their configurations. Similar to how the k8s api deprecates features.

The Kubernetes project has a well-documented deprecation policy for features. This policy states that stable APIs may only be deprecated when a newer, stable version of that same API is available and that APIs have a minimum lifetime for each stability level. A deprecated API is one that has been marked for removal in a future Kubernetes release; it will continue to function until removal (at least one year from the deprecation), but usage will result in a warning being displayed. Removed APIs are no longer available in the current version, at which point you must migrate to using the replacement.

I understand that the code base isn't currently maintainable, and that simplification and feature removal is necessary. I also think that the communication could be improved. Removal of the global-rate-limit isn't the end of the world, but I expect you'll find a lot more people were using features when things break or release notes show that ingress-nginx has been gutted.

that is a fair point and a good comment

Thanks